Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Predeploy more resources #5

Draft
wants to merge 29 commits into
base: upstream-clone
Choose a base branch
from
Draft

Conversation

c-gerke
Copy link

@c-gerke c-gerke commented Nov 25, 2024

What are you trying to accomplish with this PR?
This will allow more resources to be marked as predeployed, including Services, Jobs and Deployments. This will allow invoking Krane fewer times to deploy the same number of resources in the case where multiple Krane invocations would be necessary to deploy resources in a specific order, e.g. making sure a Redis instance is set up before a primary application without invoking Krane twice.

How is this accomplished?
This is accomplished by having the cluster resource discovery respect if the 'predeployed' annotation is set on Services, Jobs and Deployments. This includes checking if the 'predeployed' annotation is set on Services, Deployments and Jobs, and then having the resource deployer check if the predeployed annotation is set. If it is, the deploy task deploys Deployments and Services before the individual pods, and Jobs after the pods.

After all of that is said and done, Deployments, Services and Jobs will be deployed in 'Phase 3', before 'Phase 4' where the main deployment occurs.

What could go wrong?
This could unintentionally create race conditions if the annotations are set incorrectly on resources that shouldn't be predeployed, creating an out of order deployment.

Copy link
Member

@benlangfeld benlangfeld left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docs?

@c-gerke
Copy link
Author

c-gerke commented Nov 25, 2024

Docs?

I just pushed ca20352 to add some information to the README, I'm looking more to see if it would make sense to document this elsewhere!

README.md Outdated
- `krane.shopify.io/predeployed`: Causes a Custom Resource to be deployed in the pre-deploy phase.
- _Compatibility_: Custom Resource Definition
- `krane.shopify.io/predeployed`: Causes a Custom Resource, Deployment, Service, or Job to be deployed in the pre-deploy phase.
- _Compatibility_: Custom Resource Definition, Deployment, Service, Job
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be controversial upstream; the label originally got applied to the resource definition (class) not the concrete instance, so this is now conceptually mixed.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had been thinking about that as well, that this implementation/change was the result of taking a feature that existed with one purpose and kind of stealing or borrowing it to apply to different objects. I don't doubt it would be controversial upstream, and I'm ready to have those conversations on if this should even be merged upstream too!

I think it's a worthwhile feature/functional change though (I do realize I'm a little close to it to see it completely objectively), and there's at least one other person at some point who was looking for this type of functionality, so I think it's worth at least proposing and discussing if this is something that would be desired in its current state.

- Updated `predeploy_sequence` to specify resource groups for better clarity.
- Modified `predeployed?` method logic to allow for default behavior based on resource type.
- Introduced `default_to_predeployed?` method in multiple resource classes to standardize predeployment behavior.
- Removed redundant `predeployed?` methods from `Deployment`, `Job`, and `Service` classes.
- Added debug logging in integration tests for better traceability of resource groups during deployment.
This will be fixed later on, but getting a proposed version up in a PR to Shopify's Krane is the priority
lib/krane/deploy_task.rb Show resolved Hide resolved
lib/krane/deploy_task.rb Show resolved Hide resolved
lib/krane/resource_deployer.rb Show resolved Hide resolved
- Introduced a constant `PREDEPLOYED_RESOURCE_TYPES` to manage predeployed resource types.
- Updated `predeployed?` method to utilize the new constant for Role and RoleBinding checks.
- Removed redundant `predeployed?` methods from ConfigMap, NetworkPolicy, PersistentVolumeClaim, ResourceQuota, Role, RoleBinding, Secret, and ServiceAccount classes, simplifying their implementation.
- Set `default_to_predeployed?` to always return true in CustomResourceDefinition.
… on this PR

Also add CRDs and CRs to their previous functionality, not using the `default_to_predeployed` functionality
… annotation is correctly deployed in phase 3
Multiple test cases in `serial_deploy_test.rb` to verify that various Kubernetes resources (Service, Ingress, Job, DaemonSet, PodDisruptionBudget, PodTemplate, ReplicaSet, StatefulSet) with the `krane.shopify.io/predeployed` annotation are correctly predeployed and logged in the expected phases.
Remove a pedantic documentation change that increases the number of changes in this fork for little benefit

Fix link that points to the Power fork instead of Krane fork
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants